Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Allocate a guard page for each newly created thread #2545

Merged
merged 2 commits into from
May 28, 2019
Merged

Allocate a guard page for each newly created thread #2545

merged 2 commits into from
May 28, 2019

Conversation

LemonBoy
Copy link
Contributor

No description provided.

@daurnimator
Copy link
Collaborator

This PR no longer relies on the kernel's guard page (it removes MAP_GROWSDOWN) and does it in userland instead.
Doesn't this mean that the stack can no longer grow?

@LemonBoy
Copy link
Contributor Author

Doesn't this mean that the stack can no longer grow?

Yes, but MAP_GROWSDOWN it's not a good idea. You can easily see that glibc nor musl use it anymore and set-up the guard page by themselves. The main thread stack is still growable and hopefully protected by a large-enough gap.

@daurnimator
Copy link
Collaborator

daurnimator commented May 24, 2019 •
edited

MAP_GROWSDOWN it's not a good idea.

TIL.

The main thread stack is still growable

How does this work if not MAP_GROWSDOWN?
What about other threads?

@LemonBoy
Copy link
Contributor Author

How does this work if not MAP_GROWSDOWN?

The kernel is still using that flag internally (as VM_GROWSDOWN) but is extra careful in making sure there's no possibility of clashing between stacks and "common" allocations. Rust tried to allocate a guard page for the main thread stack but had to revert that choice, relying on the kernel-provided guard gap is the way to go.

What about other threads?

The whole stack is allocated and a guard page is added, a simple trick that solves so many (security) problems.

@LemonBoy LemonBoy closed this May 24, 2019
@LemonBoy LemonBoy reopened this May 24, 2019
andrewrk
andrewrk reviewed May 28, 2019
View reviewed changes
std/thread.zig
mmap_slice,
os.PROT_READ | os.PROT_WRITE,
) catch |err| switch (err) {
error.OutOfMemory => unreachable,
Copy link
Member

@andrewrk andrewrk May 28, 2019 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this truly unreachable? My understanding is that this "splits" a mapping which can potentially result in ENOMEM. Fortunately OutOfMemory is one of the possible errors of spawning a thread, so I think we can just return it.

@andrewrk andrewrk merged commit 381f845 into ziglang:master May 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andrewrk andrewrk andrewrk left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@LemonBoy @daurnimator @andrewrk